win32: Detect Cygwin/MSYS PTY #1099
Conversation
* Update to 2.0.0. * Import a patch to detect mintty from ggreer/the_silver_searcher#1099. This fixes a well known problem which has been reported many times: msys2#431, msys2/MSYS2-packages#491, etc.
When running Win32 version of Ag on mintty or other Cygwin/MSYS terminal emulators, Ag cannot detect Cygwin/MSYS pty and it causes some problems. E.g. ggreer#535 (comment) msys2/MSYS2-packages#491 Import a check routine from https://github.com/k-takata/ptycheck .
Don't know why clang-format on Travis complains this.
k-takata
force-pushed
the
detect-cygwin-pty
branch
from
5432bdc to
e1e7212
Compare
* Update to 2.0.0. * Import a patch to detect mintty from ggreer/the_silver_searcher#1099. This fixes a well known problem which has been reported many times: #431, msys2/MSYS2-packages#491, etc.
* Update to 2.0.0. * Import a patch to detect mintty from ggreer/the_silver_searcher#1099. This fixes a well known problem which has been reported many times: msys2#431, msys2/MSYS2-packages#491, etc.
|
@k-takata, I submitted #1146 without noticing this PR. One of its commits ( Would you object if we go ahead with #1146 instead of this? Do you notice any functional differences other than the ones I mentioned at #1146 (comment) ? |
Fix potential buffer overflow.
|
@k-takata ping on my last question? any reason to prefer a solution with an order of magnitude more lines of code (and affected files and build procedure), which does basically the same as a much smaller solution? |
|
I'll leave the decision to @ggreer . |
|
But it's not only about this PR for ag. You use a variant of your fix in several other places in msys2 too. It's a big chunk of code which can really be much smaller and simpler. Don't you prefer smaller if it does the same thing? @k-takata also, he didn't send the PR, you did. So I'm asking whether you think there's a reason to prefer this PR over #1146 ? |
|
Iscygpty is intended to be easily included in other software including non-MSYS2 software. The user should just include the two files into the project, add |
This is partially off topic for ag, but Microsoft doesn't support XP anymore, and as far as I know neither do Msys2 nor cygwin. Therefore, jumping through hoops to support XP is, IMO, not required, because there's no current use case where a cygwin/msys2 tty exists on XP. As for dynamic loading, it's a 5 lines function (after you remove XP support and the other redundant stuff). I don't think there's a good use case for dynamic loading. Note that the dynamic loading support is for DLLs which are used by the detection and are only required to enable the detection on XP. It's not for putting And, specifically for ag, while you might want to provide XP support and other features when you use such patch downstream (e.g. in Msys2), imposing all of this on upstream is a lot of code for an unsupported use case - msys/cygwin on XP. As a side note, and this is off topic for ag, I think the cygpty downstream msys2 patches (you mentioned vim and others) should also drop XP and simplify the code, because msys2 itself doesn't support XP anymore - again - as far as I know. |
|
At least Vim still wants to support XP, so removing such configuration is not an option for iscygpty. |
I'm pretty sure the msys2/mingw vim binary you build with the cygpty patch doesn't run on xp anyway. And even if it does, there's no value in the cygpty detection since msys2/cygwin don't run on XP. |
When running Win32 version of Ag on mintty or other Cygwin/MSYS terminal
emulators, Ag cannot detect Cygwin/MSYS pty and it causes some problems.
E.g.
#535 (comment)
msys2/MSYS2-packages#491
Import a check routine from https://github.com/k-takata/ptycheck .